Fix integer overflow in Vulkan multiply_integers#18681
Conversation
pytorch-bot
bot
added
the
module: vulkan
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/18681
Note: Links to docs will display an error until the docs builds have been completed. ❌ 8 New Failures, 2 Unrelated FailuresAs of commit 7762227 with merge base 3d2c853 ( NEW FAILURES - The following jobs have failed:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
BROKEN TRUNK - The following job failed but was present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
bot
added
the
CLA Signed
This PR needs a
|
There was a problem hiding this comment.
Pull request overview
This PR hardens the Vulkan backend’s integer multiplication utility used for tensor size/numel calculations by adding per-step overflow detection, preventing undersized GPU buffer allocations when tensor dimensions are attacker-controlled (e.g., from PTE files).
Changes:
- Add
c10::mul_overflows-based overflow checking tomultiply_integers(iterator overload). - Update the container overload to delegate to the iterator overload.
- Add the necessary
c10/util/safe_numerics.hinclude.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace std::accumulate with std::multiplies<>() with an explicit loop using safe_multiply_int64() that pre-checks for overflow before each multiplication. Prevents undersized GPU buffer allocations from attacker-controlled tensor dimensions in PTE files. Addresses TOB-EXECUTORCH-27. This PR was authored with the assistance of Claude.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| inline int64_t multiply_integers(Iter begin, Iter end) { | ||
| // std::accumulate infers return type from `init` type, so if the `init` type | ||
| // is not large enough to hold the result, computation can overflow. We use | ||
| // `int64_t` here to avoid this. | ||
| return std::accumulate( | ||
| begin, end, static_cast<int64_t>(1), std::multiplies<>()); | ||
| int64_t result = 1; | ||
| for (Iter it = begin; it != end; ++it) { | ||
| VK_CHECK_COND( | ||
| !c10::mul_overflows(result, static_cast<int64_t>(*it), &result), | ||
| "Integer overflow in multiply_integers"); | ||
| } |
There was a problem hiding this comment.
multiply_integers() now checks for int64_t multiplication overflow, but it still permits negative factors. Since tensor sizes in PTE schema are signed (schema/program.fbs:108), a malicious file could supply negative dims; the product can become negative and then be implicitly converted to size_t/uint32_t at call sites, resulting in huge allocations or other incorrect behavior. Consider explicitly rejecting negative factors (and possibly also validating result fits the intended unsigned destination type) before returning.
SS-JIA
left a comment
SS-JIA
left a comment
There was a problem hiding this comment.
Overall the intent of the change is sound. However, I want to avoid bringing a c10 dependency into Vulkan backend, if possible.
Replace std::accumulate with c10::mul_overflows to check for overflow at each multiplication.
Prevents undersized GPU buffer allocations from attacker-controlled tensor dimensions in PTE files.
This PR was authored with the assistance of Claude.
Test plan
CI
cc @SS-JIA @manuelcandales @digantdesai @cbilgin